That way it's clear this isn't meant for any other part of the app to meddle in the details of.

(And then I think the similar annotation on debugOtpOverride becomes redundant.)

… Ah I see, this type is used by _ZulipAppState.didPushRouteInformation, to call handleWebAuthUrl.

Oddly when I added this @visibleForTesting, that didn't cause any analysis error. I guess in particular that means the one on debugOtpOverride wouldn't be redundant with it after all.

From my comment at lastBuiltKey, though, I think this state type can remain private.

We can leave a brief note on how these are generated:

test [nfc]: Move prepareBoringImageHttpClient to test_images.dart

This was already being used for tests outside the test file it was
defined in, and we'd like to start using it in an additional
different test file. So, define it centrally in this place that
makes sense.

Ah good thought, thanks. I'd forgotten there was this natural home for it already.

nit: On my first reading, I thought "like Google" referred to the divider resembling something Google does.

Probably possible to reword, but I think the ", like Google" part can be left out entirely and it seems just as clear.

Or other possibilities. But I think "operational error" is likely to be opaque jargon for most users.

This fallback between the two fields gets repeated several times, so let's pull it into a little method, like _getOtp.

Then as a bonus, if you want to write this 100% the way the Flutter framework would: that method can use asserts so that outside of debug mode, the debug field never even gets read. (In a hot path, that sort of thing is important if you don't fully trust the optimizer to be able to prove that the field is always null. This is far from a hot path, so the optimization really doesn't matter.)

I'm a little curious how the Flutter framework does this, but in this case I'm not sure how exactly to do it. assert only does anything in debug mode, right, so I'm not sure what change we'd want to make to affect things outside of debug mode.

An example of what I have in mind is UpdateMachine.debugEnableRegisterNotificationToken. The key is that the assert callback sets a local variable that belongs to the outer function.

Then for authentic upstream examples, browse through framework.dart with that pattern in mind. The first one I spot from just starting to read through the Element implementation is Element.debugIsDefunct.

Interesting; thanks! I think I've got it:

  String? _otp;
  String? _getOtp() {
    String? result = _otp;
    assert(() {
      result = LoginPage.debugOtpOverride ?? _otp;
      return true;
    }());
    return result;
  }

Yeah, exactly.

I think you can also simplify slightly, deduplicating the use of _otp, by initializing to null, having the assert override that, and then using ?? _otp after that.

This code feels a bit cramped from being indented so far to the right. I think it'd benefit from pulling out a piece as a local variable.

Specifically I think the Column, with a name like loginForm, would work well.

Hmm, wacky.

Let's perhaps limit this condition more tightly, like only to iOS. That way if there's an error we didn't expect — which might mean the launch really did fail — we'll show the user an error message rather than silently do nothing.

Two other upstream reports that are directly about errors like this one:

• [url_launcher] Throw exception when clicked on Stop loading button on Safari flutter/flutter#75691
• [url_launcher] - PlatformException on ios 13.3 when opening a docx file flutter/flutter#49162

This looks like possibly the most relevant:
flutter/flutter#75691 (comment)

Does that match the situation you were seeing these errors in?

I'm not sure. In that comment, the person says "if anything fails browser side". But when I watch what goes on in the browser, nothing stands out to me as matching that description.

nit:

nit:

where otp is a local, and you refer to debugOtpOverride just to set it once.

That way this line doesn't get so long.

nit: put these in general-to-specific order, same as in the pattern above

Let's order all these members in the way suggested in the Flutter style guide:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense

I think that'll make it somewhat easier to look at this class and understand the API.

(It looks like the existing code wasn't quite in that order, and perhaps it'd be better if it were. But that matters a lot less when it's so simple; as it gains more complexity, the structure becomes more important for the reader.)

It looks like the existing code wasn't quite in that order

Hmm, really? Here's the class before the commit that adds to it:

class LoginPage extends StatefulWidget {
  const LoginPage({super.key, required this.serverSettings});

  final GetServerSettingsResult serverSettings;

  static Route<void> buildRoute({required GetServerSettingsResult serverSettings}) {
    return _LoginSequenceRoute(
      page: LoginPage(serverSettings: serverSettings));
  }

  @override
  State<LoginPage> createState() => _LoginPageState();
}

I see the following, matching the order in the style guide:

1. Constructors […]

[…]

4. Final fields that are set from the constructor.
5. Other static methods [i.e. that don't return the same type as the class].

[…]

10. Methods […]

I see that the updated class, with web-auth things added to it, doesn't match the numbered list; I'll fix that.

I see the following, matching the order in the style guide:

Oh, I guess that's right. I was reading buildRoute as returning the class's type, but it doesn't.

I think it's likely helpful to treat it for the ordering as if it does return the class's type, though. It plays pretty much the role of a factory constructor — it's the thing that application code, at least, should always use instead of calling the actual constructor directly. (Possibly we should even make the constructor private when we have a buildRoute method like this; but that might be annoying in tests.)

This is functionally a getter backed by a mutable field, so let's follow the Flutter style guide's order for those:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense

Also could make this an actual getter/field pair. E.g. String? get _otp and String? __otp;.

Interesting; yeah, I guess it's fine to have a thing that starts with two underscores. 😛

nit: break line for readability

nit: key detail is off past 80 columns, so break line:

nit:

Seems more precise about what's really being checked here. (And an OTP is random noise when it is properly generated, so the reader may wonder what it means for it to be "nonsense".)

nit: I like the name "payload" for similar locals elsewhere in this commit — I think that'd be clearer here than "result".

Let's give this a unit test, too. For a smoke test: it returns a value (rather than throw), and the value is a hex string of the right length.

And then given that a previous draft of this function elsewhere actually had a bug where the randomness was off — it had rand.nextInt(255) instead of 256 — let's have a test that would have caught that, so necessarily a probabilistic test.

We can follow the lead of our BackoffMachine test and say the probability of a false failure just needs to be under 1e-9. So…

• generate N = 216 of these
• take the set of bytes that ever appear (after decoding them all from hex)
• check that all 256 possible byte values do appear somewhere.

That should be plenty quick to run, I hope. And each possible byte value gets N * 32 opportunities to show up, each with probability 1/256; so its probability of missing all of those is exp(- N * 32 / 256) < 2e-12, and there are 256 such possible byte values so the probability that any of them gets missed is < 1e-9.

Then since we'll be generating all those sample results, may as well use those to subsume the smoke test: just check each of them has the right length (and we'll already be decoding them as hex so implicitly checking they're hex strings).

One possible failure mode of a buggy implementation could be to drop leading zero bytes, for example, and this check would catch that.

nit: process more on the result of check rather than its argument:

nit: can use Iterable.generate instead of allocating a List, and then in fact can leave out the callback argument:

Then probably clearest to just inline that.

Let's make the principle here a bit more explicit — helpful for anyone thinking of adding another probabilistic test and finding this as an example:

I did a quick test to confirm this test runs quickly, and it does. Running this test file with or without this test case included, 3 runs each, the difference in runtime is less than the noise between runs:

$ time flutter test test/api/model/web_auth_test.dart 
00:09 +8: All tests passed!                                                         
time: 10.660s wall (13.258s u, 0.864s s)

$ time flutter test test/api/model/web_auth_test.dart 
00:01 +8: All tests passed!                                                         
time: 2.808s wall (3.231s u, 0.613s s)

$ time flutter test test/api/model/web_auth_test.dart 
00:01 +8: All tests passed!                                                         
time: 2.724s wall (3.155s u, 0.596s s)

$ time flutter test test/api/model/web_auth_test.dart 
00:01 +8: All tests passed!                                                         
time: 2.702s wall (3.165s u, 0.581s s)

$ time flutter test test/api/model/web_auth_test.dart --name WebA
00:01 +7: All tests passed!                                                         
time: 2.764s wall (3.275s u, 0.580s s)

$ time flutter test test/api/model/web_auth_test.dart --name WebA
00:01 +7: All tests passed!                                                         
time: 2.759s wall (3.110s u, 0.642s s)

$ time flutter test test/api/model/web_auth_test.dart --name WebA
00:01 +7: All tests passed!                                                         
time: 2.710s wall (3.188s u, 0.568s s)

(I made one warmup run before the runs I counted, though I was surprised how big the warmup effect was.)

In particular I'm pretty sure the runtime is <50ms, which is fine.

Thanks!

OK, the changes here and in Info.plist LGTM.

(The intent filter and CFBundleUrlTypes match what we have in zulip-mobile; the other bits are what's prescribed in the docs under https://docs.flutter.dev/ui/navigation/deep-linking .)

So what remains to do on this PR is those small comments at #600 (review) on the new test, and then the bigger point from my end-to-end testing after that.